Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add dirty tracking of paranoia attributes #375

Open
wants to merge 2 commits into
base: core
Choose a base branch
from
Open

Add dirty tracking of paranoia attributes #375

wants to merge 2 commits into from

Conversation

pfeiffer
Copy link

@pfeiffer pfeiffer commented Jan 19, 2017 •
edited
Loading

This PR adds ActiveModel dirty tracking to the paranoid attributes.

See: #373

This is extremely useful if you have after_commit callbacks that should only run if certain attributes have changed, eg. if deleted_at is changed:

after_commit do
  keys_requiring_sync = %w[deleted_at email]
  if (previous_changes.keys & keys_requiring_sync).any?
    SyncUserWithExternalService.new(id).run!
  end
end

PR also supports custom attributes (eg. active and destroyed_at). In that case dirty tracking is enabled for all attributes.

@pfeiffer
Copy link
Author

Will fix and squash commits to support Rails 5.0.0.

@BenMorganIO
Copy link
Collaborator

This is awesome. Thanks @pfeiffer !

@kmcgaire
Copy link

@pfeiffer whats the status on this?

@pfeiffer
Copy link
Author

Sorry for not updating on this before now.

I didn't manage to get the dirty tracking working on Rails 5 since there has been some changes to the dirty tracking internals. The project I need this for isn't on Rails 5 yet, so it's not relevant for me.

Feel free to dig in and update make a new PR with Rails 5 support! :)

@dgmora
Copy link

dgmora commented Aug 8, 2017

@BenMorganIO It seems update_columns resets the previous changes in rails 5.1. If you add #{paranoia_column}_will_change after the update_columns, the tests pass. They don't pass with rails 5.0.x though, I couldn't find why 😢. Would you accept a PR that makes this pass for rails < 5.0 and >= 5.1 ?
Looking through rails code is a mess 😅

@BenMorganIO
Copy link
Collaborator

@dgmora I'd love to be able to look at it and maybe we can figure things out from there.

@dgmora dgmora mentioned this pull request Aug 8, 2017
Open
@dgmora
Copy link

dgmora commented Aug 14, 2017

@BenMorganIO Did you see #416 ? 🙂

@mathieujobin
Copy link
Collaborator

👀 is this still relevant to someone ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pfeiffer @BenMorganIO @kmcgaire @dgmora @mathieujobin